feat(client/stdio): opt-in process-tree teardown on close() - #2596
Open
siddheshbandgar wants to merge 1 commit into
Open
feat(client/stdio): opt-in process-tree teardown on close()#2596siddheshbandgar wants to merge 1 commit into
siddheshbandgar wants to merge 1 commit into
Conversation
StdioClientTransport.close() signals only the direct child, so servers launched through a wrapper (npx/uvx/python -m) leave the real server orphaned; on Windows ChildProcess.kill() cannot terminate a tree at all. Add an opt-in `killProcessTree` option: on POSIX the child leads its own process group (detached) and close() signals the group; on Windows teardown goes through `taskkill /T /F`. Both fall back to the plain kill. Defaults to false. Fixes modelcontextprotocol#2023
🦋 Changeset detectedLatest commit: cb22c83 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
Author
|
Hi maintainers — friendly bump on this one. 👋 This PR addresses #2023 (orphaned server processes when No pressure — but if you'd prefer a different direction (e.g. a consumer-supplied cleanup hook, different defaults/naming), happy to iterate. Would appreciate a review whenever you get a chance. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2023.
Problem
StdioClientTransport.close()signals only the direct child. When a server is launched through a wrapper —npx,uvx,python -m— the wrapper dies and the real server is orphaned. On WindowsChildProcess.kill()cannot terminate a tree at all.Change
Adds an opt-in
killProcessTreeoption toStdioServerParameters(Options A/B from the issue, combined):detached: trueso it leads its own process group, andclose()signals-pid, reaching every descendant.taskkill /pid <pid> /T /F.proc.kill(signal)if the group/taskkill call fails.Default is
false, so signal propagation and Ctrl-C behaviour are unchanged for anyone who doesn't opt in — this is deliberate, sincedetached: trueby default would turn a crashed host into a permanent orphan factory (the trade-off called out as Option B in the issue).Test
stdioKillProcessTree.test.tsspawns a wrapper that forks a long-lived grandchild, records its pid, closes the transport, and asserts the grandchild is reaped. Skipped on Windows in CI; thetaskkillpath was verified manually.Happy to change the default, or move to Option C (a consumer-supplied cleanup hook), if you'd prefer a different direction.
A changeset is included (
client: minor).